Skip to content

chore(deps): switch from fast-glob to tinyglobby #6354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

chore(deps): switch from fast-glob to tinyglobby #6354

wants to merge 1 commit into from

Conversation

@benmccann
Copy link
Contributor

🎉 Thanks for submitting a pull request! 🎉

Summary

https://npmgraph.js.org/?q=fast-glob - 17 dependencies
https://npmgraph.js.org/?q=tinyglobby - 2 dependencies

I see #6094 tried to switch both fast-glob and glob. However, tinyglobby is currently only a replacement for globby and fast-glob. I made a request for glob support awhile back here: SuperchupuDev/tinyglobby#97. It might not be implemented anytime soon if at all, so it'd be nice to migrate just fast-glob in the meantime, which is still a nice win in terms of dependency size.

For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures
    we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
    something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures
    your code follows our style guide and passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@benmccann benmccann requested a review from a team as a code owner May 21, 2025 16:38
@serhalp serhalp added the type: chore work needed to keep the product and development running smoothly label May 26, 2025
@mrstork
Copy link
Contributor

mrstork commented Jun 4, 2025 •
edited
Loading

Hello @benmccann, I've just merged #6384 which replacing use of glob with fast-glob, so there are a few more occurrences to replace with tinyglobby. Now that we're only making use of one glob library, replacement should be even more impactful 👍

@benmccann benmccann force-pushed the tinyglobby branch 3 times, most recently from 7f62758 to 475861c Compare June 5, 2025 20:15
@benmccann
Copy link
Contributor Author

Unfortunately #6384 made this much harder because it introduced the use of the globstar option, which is not present in tinyglobby. While I haven't investigated in detail, I suspect that's why the tests are failing. I'll file a request in that repo and see if the author has any ideas in terms of work around or whether it's hard to implement and will reference this issue, so you can find it

Failing tests ``` ❯ tests/symlinked_included_files.test.ts:59:47 57| 58| // expect that the symlink for `node_modules/crazy-dep` is preserved 59| expect(await readDirWithType(unzippedPath)).toEqual({ | ^ 60| '___netlify-bootstrap.mjs': false, 61| '___netlify-entry-point.mjs': false,

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/3]⎯

FAIL tests/symlinked_included_files.test.ts > preserves multiple symlinks that link to the same target
AssertionError: expected { …(10) } to deeply equal { …(14) }

  • Expected

  • Received

    {
    "___netlify-bootstrap.mjs": false,
    "___netlify-entry-point.mjs": false,
    "___netlify-telemetry.mjs": false,
    "function.mjs": false,

❯ tests/symlinked_included_files.test.ts:109:59
107|
108| // Test to be sure we've made both symlinks, not just one of them
109| expect(await readDirWithType(join(tmpDir, 'function'))).toEqual({
| ^
110| '___netlify-bootstrap.mjs': false,
111| '___netlify-entry-point.mjs': false,

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/3]⎯

FAIL tests/symlinked_included_files.test.ts > symlinks in subdir of includedFiles are copied over successfully
AssertionError: expected { …(5) } to deeply equal { …(6) }

  • Expected

  • Received

    {
    "___netlify-bootstrap.mjs": false,
    "___netlify-entry-point.mjs": false,
    "___netlify-telemetry.mjs": false,
    "function.cjs": false,

  • "subproject/node_modules/.bin/cli.js": true,
    "subproject/node_modules/tool/cli.js": false,
    }

❯ tests/symlinked_included_files.test.ts:153:59
151| })
152|
153| expect(await readDirWithType(join(tmpDir, 'function'))).toEqual({
| ^
154| '___netlify-bootstrap.mjs': false,
155| '___netlify-entry-point.mjs': false,

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[3/3]⎯

</details>

@benmccann benmccann mentioned this pull request Jun 7, 2025
Closed
SuperchupuDev
SuperchupuDev reviewed Jun 7, 2025
View reviewed changes
packages/zip-it-and-ship-it/src/runtimes/node/utils/included_files.ts Outdated
expandDirectories: false,
})

// tinyglobby ends directories with `/`, so we can filter them out using that
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi this is not guaranteed and the opposite can be true in rare (but known) cases. probably best to not rely on trailing /

@benmccann benmccann force-pushed the tinyglobby branch 6 times, most recently from 2094083 to bcc4106 Compare June 18, 2025 17:32
@benmccann benmccann mentioned this pull request Jun 18, 2025
Open
@benmccann
Copy link
Contributor Author

@mrstork I've implemented the globstar option in tinyglobby, but the CI is still failing because it turns out there's a bug in tinyglobby: SuperchupuDev/tinyglobby#133. It's not the easiest bug to fix because it likely requires new functionality to be implemented in the underlying fdir library. Before I do that, I wanted to check with you to understand exactly what this code is doing and ensure there's not an easier solution

Are you familiar with included_files.ts and what it's doing with regards to symlinks? It looks like it's collecting the links, but not traversing into them - is this necessary and how does it fit into what this package is doing? I don't have a lot of familiarity with this package, so any background information you can share as part of the explanation would be helpful.

@mrstork
Copy link
Contributor

mrstork commented Jul 3, 2025

@benmccann All of this predates me by quite a bit, but I'll do my best.

At a high level though, zip-it-and-ship-it (or zisi) is responsible for packaging the necessary pieces of a user's codebase for use in Netlify Functions, or more specifically AWS Lambdas. For more context, I recommend reading the project README, reading through the comments in the code paths you're editing, and reading through the failing tests (as well as the comments in those tests to understand why they are there).

From what I know, none of this code is extraneous. If a test is failing, it's necessary for it to pass in order for the code changes to be merged. As time/capacity allows, I can look to dig in here with you to try to come up with an alternative solutions.

@benmccann benmccann marked this pull request as draft August 12, 2025 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@SuperchupuDev SuperchupuDev SuperchupuDev left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

type: chore work needed to keep the product and development running smoothly

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benmccann @mrstork @SuperchupuDev @serhalp